Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix two bugs related to tasks #12195

Merged
merged 14 commits into from
Sep 29, 2021
Merged

Fix two bugs related to tasks #12195

merged 14 commits into from
Sep 29, 2021

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Sep 27, 2021

This fixes two problems reported for tasks

#12189 is bad codegen for generic tasks inside a generic class. It's fixed by more careful coding in IlxGen.fs

#12188 is a more subtle fix. The bug stems from the fact that our processing of SRTP constraints for generic task code is unfortunately hitting known problems with processing SRTP constraints. This only happens in situations where task { .. } code is underspecified w.r.t. the types involved in binding, e.g.

let myFunction f =
    task {
        do! f ()
        return ()
    }

This code is not wrong - however its ambiguity can currently cause later inference problems. In particular, the return type of f is not known to be a task, nor is it known to be any of the other available binding types (Task, "Task like" or Async), leaving the type inference SRTP constraints unsolved, and possibly resolved later in the file. This ambiguity can cause later problems, described below.

The workaround is always to make the types being bound immediately clear:

let myFunction (f: unit-> Task<_>) =
    task {
        do! f ()
        return ()
    }

The underlying problem goes right back to the mis-inclusion of #1650 into F#. In short #1650 got (very rashly) included in F# 4.0, we backed it out, but people had already started to take dependencies on it and reinstated it for stability. #1650 effectively introduced a new rule into F# type checking along the lines "If adding a constraint means a type inference problem gives rise to an unresolved SRTP constraint, then continue without further processing the constraint". The unfortunate effect of this has always been that, in some situations, constraints are not fully applied to F# type checking - constraint solving is "aborted" when SRTP constraints can't be solved. Normally this isn't a problem because, for various reasons, the constraints are re-solved later. However that doesn't always happen, and indeed #12188 is one such case, and the end result is bad code generation.

Now, the "obvious" fix is to always fully-applying the constraints and remove #1650. However this changes the order of type inference which can affect name resolution and other type checking results.

This systematically fixes this problem without changing type inference order, by

  1. saving ("postponing") the constraints when Speed up Constraint Solver's overload resolution (II) #1650 kicks in
  2. applying them at the end of inference, before defaults are applied.

This fixes the immediate problem for #12188

@dsyme dsyme changed the title [WIP] fix srtp processing related to tasks Fix two bugs related to tasks Sep 28, 2021
…nd put it in the constraint solver context instead
…nd put it in the constraint solver context instead
…nd put it in the constraint solver context instead
@Happypig375
Copy link
Member

@gusty for original submitter

@@ -2976,7 +3093,7 @@ and ResolveOverloading
calledMethOpt,
trackErrors {
do! errors
let cxsln = Option.map (fun traitInfo -> (traitInfo, MemberConstraintSolutionOfMethInfo csenv.SolverState m calledMeth.Method calledMeth.CalledTyArgs)) cx
let cxsln = AssumeMethodSolvesTrait csenv cx m trace calledMeth
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a lot clearer now.

@dsyme
Copy link
Contributor Author

dsyme commented Sep 29, 2021

@TIHan @vzarytovskii I pushed one additional update to this to make sure we pop the actions if there is an undo-trace active - will merge when green

@vzarytovskii
Copy link
Member

@dsyme we'll probably need to backport it to dev17 branch too, if we want it in SDK and VS.

CC @TIHan @brettfo

@dsyme
Copy link
Contributor Author

dsyme commented Sep 29, 2021

@vzarytovskii yes absolutely. I'll merge now and set up the back port tomorrow

@dsyme dsyme merged commit 2897230 into dotnet:main Sep 29, 2021
dsyme added a commit to dsyme/fsharp that referenced this pull request Sep 30, 2021
* fix srtp processing related to tasks

* fix 12189 - bad codegen for tasks. Also eliminate 'trace' parameter and put it in the constraint solver context instead

* fix 12189 - bad codegen for tasks. Also eliminate 'trace' parameter and put it in the constraint solver context instead

* fix 12189 - bad codegen for tasks. Also eliminate 'trace' parameter and put it in the constraint solver context instead

* fix 12189 - bad codegen for tasks. Also eliminate 'trace' parameter and put it in the constraint solver context instead

* cleanup and fix method arg lambda propagation rule

* fix error messages

* fix error messages

* fix error messages

* fix error messages

* simplify diff

* simplify diff

* reduce diff and fix errors

* reduce diff and fix errors
dsyme added a commit that referenced this pull request Sep 30, 2021
* fix srtp processing related to tasks

* fix 12189 - bad codegen for tasks. Also eliminate 'trace' parameter and put it in the constraint solver context instead

* fix 12189 - bad codegen for tasks. Also eliminate 'trace' parameter and put it in the constraint solver context instead

* fix 12189 - bad codegen for tasks. Also eliminate 'trace' parameter and put it in the constraint solver context instead

* fix 12189 - bad codegen for tasks. Also eliminate 'trace' parameter and put it in the constraint solver context instead

* cleanup and fix method arg lambda propagation rule

* fix error messages

* fix error messages

* fix error messages

* fix error messages

* simplify diff

* simplify diff

* reduce diff and fix errors

* reduce diff and fix errors
@JonCanning
Copy link

Would this fix this issue where f is always unit -> Task<unit>?

image

@dsyme
Copy link
Contributor Author

dsyme commented Oct 6, 2021

@JonCanning If you have the given type annotation, the type is Task, as you'd expect.

After this fix, you can type-annotate the other bindable types (Async<T> and any Task-like), e.g.

let myFunction (f: unit -> Async<_>) =
    task {
        return! f ()
    }

If there is no strongly known type at the resolution of return! then the default is Task<_>

> let myFunction f =
-     task {
-         return! f ()
-     };;
val myFunction: f: (unit -> #Task<'b>) -> Task<'b>

@JonCanning
Copy link

JonCanning commented Oct 6, 2021

In my example it's adding the if then that causes it to always be unit -> Task<unit>

@dsyme
Copy link
Contributor Author

dsyme commented Oct 6, 2021

@JonCanning Ok. I checked all the following on latest bits after this fix and the inference now works as expected

open System.Threading.Tasks

let myFunction value (f: unit -> Async<_>) =
    task {
        if value then do! Task.Delay 1000
        return! f ()
    }

let myFunction value (f: unit -> Task<_>) =
    task {
        if value then do! Task.Delay 1000
        return! f ()
    }

let myFunction value f =
    task {
        if value then do! Task.Delay 1000
        return! f ()
    }

give

val myFunction: value: bool -> f: (unit -> Async<'a>) -> Task<'a>
val myFunction: value: bool -> f: (unit -> Task<'a>) -> Task<'a>
val myFunction: value: bool -> f: (unit -> #Task<'b>) -> Task<'b>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants